Neutralize formula characters in csv exports#4128
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCSV conversion now neutralizes string values beginning with formula-triggering characters by prefixing them with a single quote before parsing. Non-string values continue through the existing default parsing path. ChangesCSV Value Sanitization
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/lib/actions/partners/update-partner-profile.ts`:
- Around line 33-38: The partner profile schema’s name validation has the same
required-then-sanitized ordering problem as the onboarding schema, so a
formula-only input can become empty after sanitization and overwrite the
existing value. Update the `update-partner-profile.ts` schema `name` field (and
align it with `onboardPartnerSchema.name` if shared) so `sanitizeFormulaInput`
runs before enforcing non-empty content, or add a post-transform
`refine`/equivalent check after `sanitizeFormulaInput` to reject empty results
while preserving the optional behavior.
In `@apps/web/lib/zod/schemas/partners.ts`:
- Around line 752-760: The sanitizeFormulaInput helper currently removes formula
prefixes before leading whitespace, so values like “ =...” can still end up
looking like formulas. Update sanitizeFormulaInput to strip leading whitespace
first or include it in the same leading removal, then ensure the name,
description, and companyName transforms in partners.ts use this helper so all
three fields get the corrected sanitization.
- Line 770: The name field validation in the partners schema allows values that
become empty after sanitizeFormulaInput because z.string().min(1) runs before
the transform. Update the name pipeline in the partners schema to revalidate
after sanitizeFormulaInput, matching the companyName pattern, so the sanitized
result must still be non-empty.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ac186c41-29c9-4db4-83dc-8a2bcc8dbd75
📒 Files selected for processing (2)
apps/web/lib/actions/partners/update-partner-profile.tsapps/web/lib/zod/schemas/partners.ts
There was a problem hiding this comment.
♻️ Duplicate comments (3)
apps/web/lib/zod/schemas/partners.ts (2)
752-760: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winFormula-char strip still bypassable via leading whitespace.
replace(/^[=+\-@\t\r]+/, "")only matches at index 0 and the class excludes plain space/newline, so a leading space blocks the match;.trimStart()then removes that space and re-exposes the formula trigger (" =cmd|calc!A0"→"=cmd|calc!A0"). This is the exact bug flagged in a prior review on this file and marked "Addressed", but the code here is unchanged from the vulnerable version —descriptionandcompanyName(which don't get a.trim()upstream) remain exploitable, defeating the purpose of this PR.🐛 Proposed fix
- return value.replace(/^[=+\-@\t\r]+/, "").trimStart() as T; + return value.replace(/^[\s=+\-@]+/, "") as T;Merging whitespace into the same leading character class strips interleaved whitespace/formula-char runs (e.g.
" = A1") in one pass instead of two ordered passes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/lib/zod/schemas/partners.ts` around lines 752 - 760, The sanitizeFormulaInput helper still allows a leading space/newline to hide a formula trigger because the regex only strips characters at index 0 and trimStart runs afterward. Update sanitizeFormulaInput in partners.ts so the leading-stripping step handles whitespace and formula-leading characters together in one pass, and keep the behavior applied to fields like description and companyName that do not get trimmed upstream.
770-770: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRevalidate
nameafter sanitizing.
z.string().min(1)runs before.transform(sanitizeFormulaInput), so a submission like"="passes the required check and is then sanitized to"", silently bypassing the "Name is required" constraint. This matches a still-open concern from a prior review pass on this exact line.🐛 Proposed fix
- name: z.string().min(1, "Name is required").transform(sanitizeFormulaInput), + name: z + .string() + .min(1, "Name is required") + .transform(sanitizeFormulaInput) + .refine((v) => v.length > 0, { message: "Name is required" }),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/lib/zod/schemas/partners.ts` at line 770, The `name` field in the `partners` Zod schema is only validated before `sanitizeFormulaInput` runs, so a value like "=" can become empty after transformation and bypass the required check. Update the `name` pipeline in the partners schema to validate again after `sanitizeFormulaInput` (for example by moving the required validation to the sanitized result or adding a post-transform refinement) so the final value cannot be empty. Use the `name` schema entry in `partners.ts` as the location to adjust.apps/web/lib/actions/partners/update-partner-profile.ts (1)
33-38: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSame required-then-emptied ordering issue as onboarding schema's
namefield.
.min(1, "Name is required")runs before.transform(sanitizeFormulaInput). A submittednameof purely formula-trigger characters (e.g."=") passes the required check, then sanitizes to"", silently overwriting the partner's existing name. Same class of issue asapps/web/lib/zod/schemas/partners.ts'sonboardPartnerSchema.name, previously flagged here and still unresolved.🐛 Proposed fix
name: z .string() .trim() .min(1, "Name is required") .optional() - .transform(sanitizeFormulaInput), + .transform(sanitizeFormulaInput) + .refine((v) => v === undefined || v.length > 0, { + message: "Name is required", + }),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/lib/actions/partners/update-partner-profile.ts` around lines 33 - 38, The partner profile `name` schema in `update-partner-profile.ts` has the same required-then-sanitized ordering bug as `onboardPartnerSchema.name`: the `z.string().trim().min(1, "Name is required").optional().transform(sanitizeFormulaInput)` chain validates before sanitizing, so formula-only input can become empty after transform and overwrite the existing name. Update this field so `sanitizeFormulaInput` runs before the non-empty validation, matching the fix pattern used for the onboarding schema, and keep the `name` field in sync with the corresponding partner schema definitions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@apps/web/lib/actions/partners/update-partner-profile.ts`:
- Around line 33-38: The partner profile `name` schema in
`update-partner-profile.ts` has the same required-then-sanitized ordering bug as
`onboardPartnerSchema.name`: the `z.string().trim().min(1, "Name is
required").optional().transform(sanitizeFormulaInput)` chain validates before
sanitizing, so formula-only input can become empty after transform and overwrite
the existing name. Update this field so `sanitizeFormulaInput` runs before the
non-empty validation, matching the fix pattern used for the onboarding schema,
and keep the `name` field in sync with the corresponding partner schema
definitions.
In `@apps/web/lib/zod/schemas/partners.ts`:
- Around line 752-760: The sanitizeFormulaInput helper still allows a leading
space/newline to hide a formula trigger because the regex only strips characters
at index 0 and trimStart runs afterward. Update sanitizeFormulaInput in
partners.ts so the leading-stripping step handles whitespace and formula-leading
characters together in one pass, and keep the behavior applied to fields like
description and companyName that do not get trimmed upstream.
- Line 770: The `name` field in the `partners` Zod schema is only validated
before `sanitizeFormulaInput` runs, so a value like "=" can become empty after
transformation and bypass the required check. Update the `name` pipeline in the
partners schema to validate again after `sanitizeFormulaInput` (for example by
moving the required validation to the sanitized result or adding a
post-transform refinement) so the final value cannot be empty. Use the `name`
schema entry in `partners.ts` as the location to adjust.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cdf4a93b-2563-4173-82e4-c1b6ddc3e97d
📒 Files selected for processing (2)
apps/web/lib/actions/partners/update-partner-profile.tsapps/web/lib/zod/schemas/partners.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/lib/analytics/utils/convert-to-csv.ts`:
- Around line 3-5: Update FORMULA_TRIGGER_CHARS used by neutralizeFormula to
include the line-feed character \n alongside the existing triggers, and add
regression coverage verifying values beginning with \n are prefixed with the
neutralizing apostrophe.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8e77418a-e048-47ea-b5e5-a699eb0b6cf6
📒 Files selected for processing (1)
apps/web/lib/analytics/utils/convert-to-csv.ts
|
@coderabbitai any breaking change? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Target file structure =="
ast-grep outline apps/web/lib/analytics/utils/convert-to-csv.ts --items all || true
echo "== PR diff against merge base =="
git diff --no-ext-diff --unified=40 "$(git merge-base HEAD origin/main)" -- apps/web/lib/analytics/utils/convert-to-csv.ts
echo "== Converter definitions and call sites =="
rg -n -C 3 --glob '*.{ts,tsx,js,jsx}' '\b(convertToCsv|convert-to-csv)\b' appsLength of output: 8233
There is an intentional CSV-output behavior change across all consumers of this utility: string fields beginning with The only compatibility consideration is for downstream consumers that parse the raw CSV and expect exact original values; they will receive the apostrophe-prefixed value for those edge cases. |
2 participants
Summary by CodeRabbit